Move a few components to es6 with mixin decorator #2439
Conversation
|
awaiting a conclusion on #2437. |
|
@subjectix I think that we can merge this one. |
|
@oliviertassinari No need to worry we'll iteratively merge master with this branch to keep it updated. |
|
|
||
| @autobind |
There was a problem hiding this comment.
Please use arrow functions instead of autobind.
There was a problem hiding this comment.
Instead of the arrow functions I could alternatively just leave react-codemod alone; it automatically puts this bindings in the constructor. Which would you prefer?
There was a problem hiding this comment.
Having a look at https://github.com/callemall/material-ui/pull/1734/files, I would say, let's let react-codemod puts this.
There was a problem hiding this comment.
I'm sorry but I completely disagree! Doing that makes the components extremely hard to maintain! I had one hell of bad time with leftnav's doc page component. This is not a good pattern. and using arrow would make it look like this:
_foo = () => {
// ...
};It's not so bad, is it?
There was a problem hiding this comment.
@subjectix react-router seems to follow this convention too (https://github.com/rackt/react-router/blob/master/examples/huge-apps/components/GlobalNav.js).
I don't know why they and facebook do so, but I feel like we can trust them.
|
I just updated with a version that leaves react-codemod's |
|
I seriously disagree with This makes maintenance a living hell. please don't go down this path 🙏 🙏 |
I agree with you. |
|
@oliviertassinari I can think of one thing: named functions, they help debugging. but that's not a priority with a library like this! this library changes very fast. Maintainability is far more important that some tiny extra information in the stack trace. I use them with my project and I'm truly happy with them. |
|
I personally like the es7 stage-1 decorators are still pretty new so I'm not surprised a ton of libraries aren't using them yet, though I have found uses in the wild. Personally, I use them for my React project (as well as other decorators, unrelated) and I find them to be easy to understand and keep the function declaration syntax consistent. |
|
But in the end I will defer to whatever the collaborators decide on. |
|
@BobertForever Go on and use |
|
Sounds like a plan to me :) (And I'll join you on the tears, I have so many of these decorators all around my codebase at this point) |
|
Just updated the diff back to using the decorator |
|
I'm happy with this 👍 Thanks a lot @BobertForever 🎉 @oliviertassinari Please merge if it's ok. |
|
@BobertForever Thanks for working on this. |
Move a few components to es6 with mixin decorator
|
🎉 🎉 Our first shot at this 👍 |
This is a redirection of #2436 to its own branch.